Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client certificates option to cargo #10630

Closed

Conversation

ETKNeil
Copy link

@ETKNeil ETKNeil commented May 4, 2022

This PR focus on adding a functionality that will allow to use client certificate when dealing with private registry. This allow to secure the front end of the private registry with a client certificate while still being able to communicate the api via cargo.

Eg in $HOME/.cargo/config

[http]
client-ssl-cert = "/etc/ssl/client.pem"
client-ssl-key = "/etc/ssl/key.pem"
client-ssl-key-password = "<password>"

Eg in $HOME/.cargo/config
```toml
[http]
client-ssl-cert = "/etc/ssl/client.pem"
client-ssl-key = "/etc/ssl/key.pem"
client-ssl-key-password = "<password>"
```
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2022
Comment on lines +2120 to +2122
pub client_ssl_cert: Option<ConfigRelativePath>,
pub client_ssl_key: Option<ConfigRelativePath>,
pub client_ssl_key_password: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see a linked issue or discussion. We are wanting to discuss problems and solutions in Issues before moving on to implementation in PRs. This helps make sure we are all on the same page and reduces the pressure on the tight resources of the cargo team. For example, I would want more experienced people than I involved in the conversation about how the password should be handled to be both usable and secure.

We encourage people to discuss their design before hacking on code. This gives the Cargo team a chance to know your idea more. Sometimes after a discussion, we even find a way to solve the problem without coding! Typically, you file an issue or start a thread on the internals forum before submitting a pull request. Please read the process of how features and bugs are managed in Cargo.

https://doc.crates.io/contrib/process/working-on-cargo.html#before-hacking-on-cargo

See also https://blog.rust-lang.org/inside-rust/2022/03/31/cargo-team-changes.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also encourage following the PR template as that covers things like what the testing strategy for the implementation looks like.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made an issue explaining the design decisions, but to be honest the password one is impossible to solve without building a proper password manager in cargo.
Let me just point out that registry credentials are stored in plain text as well.

@ehuss
Copy link
Contributor

ehuss commented Oct 1, 2022

I'm going to close as I don't think this is going to make progress as a PR. I think #10641 (comment) has some good questions, and indicates that the design isn't as trivial as just passing some values to curl. Considering that changes to authentication has been undergoing recent changes, and all of those have been going through the RFC process, I would suspect this might also need something similar.

@ehuss ehuss closed this Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants